Expose counterparty forwarding info in ChannelDetails #841

valentinewallace · 2021-03-12T22:18:33Z

Supersedes #207. Tested in the sample :)

codecov · 2021-03-12T22:35:56Z

Codecov Report

Merging #841 (c318ad8) into main (2cb5b1a) will increase coverage by 0.58%.
The diff coverage is 87.37%.

@@            Coverage Diff             @@
##             main     #841      +/-   ##
==========================================
+ Coverage   90.64%   91.22%   +0.58%     
==========================================
  Files          50       51       +1     
  Lines       26843    29055    +2212     
==========================================
+ Hits        24331    26506    +2175     
- Misses       2512     2549      +37

Impacted Files	Coverage Δ
lightning-net-tokio/src/lib.rs	`76.42% <0.00%> (-0.28%)`	⬇️
lightning/src/ln/msgs.rs	`90.46% <ø> (+1.84%)`	⬆️
lightning/src/ln/channelmanager.rs	`83.50% <50.00%> (+0.14%)`	⬆️
lightning/src/ln/peer_handler.rs	`48.93% <50.00%> (+4.10%)`	⬆️
lightning/src/ln/channel.rs	`90.31% <88.88%> (+3.04%)`	⬆️
lightning/src/routing/router.rs	`96.96% <100.00%> (+0.01%)`	⬆️
lightning/src/util/test_utils.rs	`83.65% <100.00%> (+0.05%)`	⬆️
lightning/src/util/events.rs	`23.89% <0.00%> (-0.44%)`	⬇️
lightning/src/util/scid_utils.rs	`100.00% <0.00%> (ø)`
lightning/src/ln/functional_tests.rs	`96.96% <0.00%> (+0.06%)`	⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2cb5b1a...c318ad8. Read the comment docs.

lightning/src/ln/channelmanager.rs

lightning/src/ln/channel.rs

ariard

Thanks to resuscitate this, almost good to me

ariard · 2021-03-16T18:27:47Z

lightning/src/ln/channel.rs

+	pub fn channel_update(&mut self, msg: &msgs::ChannelUpdate) -> Result<(), ChannelError> {
+		let usable_channel_value_msat = (self.channel_value_satoshis - self.counterparty_selected_channel_reserve_satoshis) * 1000;
+		if msg.contents.htlc_minimum_msat >  usable_channel_value_msat {
+			return Err(ChannelError::Close("Minimum htlc value is greater than channel value".to_string()));


I think this check is purposeful to make sure when we initiate channel with respect our own config or that other counterparty don't waste time/fees with an unroutable channel, but better to move it in new_from_req/new_outbound, not already present ?

The check is present in new_from_req (new_outbound isn't parameterized by htlc_min_msat). Why wouldn't we also check here, though? (Fyi, you wrote this check originally 😛)

ariard · 2021-03-16T18:40:23Z

lightning/src/ln/channel.rs

@@ -281,6 +281,13 @@ impl HTLCCandidate {
 	}
 }

+#[derive(Clone)]
+pub struct CounterpartyForwardingInfo {


I had a look on #207, effectively this make sense to learn counterparty forwarding policy on this link to communicate a routing hint to a payee. I think this struct should document this usage, as otherwise it's a bit confusing why we need to observe peer's channel_update on a local channel.

Should we also add htlc_maximum_msat ? Don't issue an invoice with an amount above max otherwise your hint is useless ?

struct should document this usage, as otherwise it's a bit confusing why we need to observe peer's channel_update on a local channel.

Documented!

Should we also add htlc_maximum_msat ? Don't issue an invoice with an amount above max otherwise your hint is useless ?

Hm, but with MPP, the total invoice amount may violate an individual channel's htlc_max_msat but not the single payment path that uses said channel. 🤔

TheBlueMatt

Looks good, a few doc nits, but dont bother unless you think you should or you're addressing other comments.

lightning/src/ln/channel.rs

lightning/src/ln/channelmanager.rs

jkczyz

Mostly nits but wondering if failures handling in the router should dictate if the channel manager should still handle the message. See comment.

lightning/src/ln/channel.rs

lightning/src/ln/peer_handler.rs

jkczyz · 2021-03-17T16:17:51Z

lightning/src/ln/peer_handler.rs

+				self.message_handler.chan_handler.handle_channel_update(&peer.their_node_id.unwrap(), &msg);
 				let should_forward = match self.message_handler.route_handler.handle_channel_update(&msg) {


If checks in route_handler fail, should we still send this message to chan_handler?

We'd need some way to differentiate "we haven't seen a channel_announcement/the channel is private" as a failure from "invalid signature" or other failures. We /could/ check the signature in ChannelManager, but we know it came directly from our peer, so not sure if we need to.

I see what you are saying for this particular message. Though I do wonder more broadly if we are expecting custom handler implementations to re-implement all BOLT checks or if there is some subset we should check regardless of implementation. Definitely outside the scope of this PR.

Yea, thats a good question. I kinda figure there's relatively limited uses for implementing a full custom message handler, and its more about the message handlers being optional/dummy in cases where, eg, you don't want to have a network graph so I was never too worried about it.

I suppose the wrapper approach in a similar vein as Read that I had mentioned elsewhere may be sufficient.

lightning/src/ln/channel.rs

lightning/src/ln/msgs.rs

This will be filled in in upcoming commits, then exposed in ChannelDetails to allow constructing route hints for invoices. Also update the cltv_expiry_deta comment in msgs::ChannelUpdate Co-authored-by: Valentine Wallace <[email protected]> Co-authored-by: Antoine Riard <[email protected]>

This will be used to expose forwarding info for route hints in the next commit. Co-authored-by: Valentine Wallace <[email protected]> Co-authored-by: Antoine Riard <[email protected]>

Useful for constructing route hints for private channels in invoices. Co-authored-by: Valentine Wallace <[email protected]> Co-authored-by: Antoine Riard <[email protected]>

valentinewallace · 2021-03-17T22:25:38Z

CI passed ;)

TheBlueMatt

Only changes since Jeff's ACK were two small doc fixes.

valentinewallace force-pushed the 207-replacement branch from 7bb9bfb to 9943259 Compare March 12, 2021 22:23

valentinewallace mentioned this pull request Mar 12, 2021

Implement local channel tracking by Router at channel_update #207

Closed

TheBlueMatt reviewed Mar 12, 2021

View reviewed changes

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved

valentinewallace force-pushed the 207-replacement branch from 9943259 to d82da70 Compare March 13, 2021 19:57

TheBlueMatt reviewed Mar 15, 2021

View reviewed changes

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved

valentinewallace force-pushed the 207-replacement branch from d82da70 to 98f5907 Compare March 15, 2021 18:45

TheBlueMatt reviewed Mar 16, 2021

View reviewed changes

lightning/src/ln/channel.rs Show resolved Hide resolved

ariard reviewed Mar 16, 2021

View reviewed changes

valentinewallace force-pushed the 207-replacement branch 2 times, most recently from 9218c57 to aaf5c4f Compare March 16, 2021 19:10

jkczyz self-requested a review March 16, 2021 22:17

TheBlueMatt approved these changes Mar 17, 2021

View reviewed changes

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

jkczyz reviewed Mar 17, 2021

View reviewed changes

valentinewallace force-pushed the 207-replacement branch 4 times, most recently from c26b951 to 84836d5 Compare March 17, 2021 19:30

jkczyz approved these changes Mar 17, 2021

View reviewed changes

TheBlueMatt reviewed Mar 17, 2021

View reviewed changes

lightning/src/ln/msgs.rs Outdated Show resolved Hide resolved

valentinewallace force-pushed the 207-replacement branch from 67c7989 to 4547cab Compare March 17, 2021 21:20

valentinewallace and others added 3 commits March 17, 2021 17:36

Pass channel updates to ChannelManager and Channel.

e8a0824

This will be used to expose forwarding info for route hints in the next commit. Co-authored-by: Valentine Wallace <[email protected]> Co-authored-by: Antoine Riard <[email protected]>

Expose counterparty forwarding info in ChannelDetails.

c318ad8

Useful for constructing route hints for private channels in invoices. Co-authored-by: Valentine Wallace <[email protected]> Co-authored-by: Antoine Riard <[email protected]>

valentinewallace force-pushed the 207-replacement branch from 4547cab to c318ad8 Compare March 17, 2021 21:36

TheBlueMatt approved these changes Mar 17, 2021

View reviewed changes

TheBlueMatt merged commit 32f6205 into lightningdevkit:main Mar 17, 2021

TheBlueMatt mentioned this pull request May 9, 2021

Redo local channel tracking for route generation #149

Closed

		self.message_handler.chan_handler.handle_channel_update(&peer.their_node_id.unwrap(), &msg);
		let should_forward = match self.message_handler.route_handler.handle_channel_update(&msg) {

Expose counterparty forwarding info in ChannelDetails #841

Expose counterparty forwarding info in ChannelDetails #841

Uh oh!

Conversation

valentinewallace commented Mar 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Mar 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

valentinewallace Mar 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jkczyz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

valentinewallace commented Mar 17, 2021

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

valentinewallace commented Mar 12, 2021 •

edited

Loading

codecov bot commented Mar 12, 2021 •

edited

Loading

valentinewallace Mar 16, 2021 •

edited

Loading